-
-
Notifications
You must be signed in to change notification settings - Fork 152
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
PR: Fix bug with environ handling #340
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @larsoner for finding the bug and checking into this! I left a comment regarding the ValueError
exception (maybe we shoudl create a new custom exception following the spirit of PythonQtError
and PythonQtWarning
).
Also, could be worthy to try to add a test to check that we now have the proper behavior (the env var QT_API
is updated correctly when is not initially set). I think the test should go in qtpy/tests/test_main.py
.
Besides that this LGTM 👍
The only clean way I could think of to do it was to use |
Green! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your work on this @larsoner! I left some small comments for you below.
Co-authored-by: Carlos Cordoba <ccordoba12@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @larsoner for your help with this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for all the work here @larsoner !
Happy to contribute to this awesome package! We've successfully used it to seamlessly transition three packages from PyQt5 only to framework-agnostic code |
Pretty cool @larsoner! If possible, could let us know what packages exactly? That help us for grant applications. |
I'm not 100% sure what the intention of modifying
os.environ
at all is, but assuming it's meant to set it so that e.g. new processes that spawn make use of the same framework, then there's a bug in the current implementation inmaster
:This bug was found precisely because a pattern like this: have no QT_API set, import PyQt6, then import qtpy, then spawn a new process (using joblib), have it fail because in that process
import qtpy
fails because PyQt5 isn't installed, butos.environ['QT_API'] == 'pyqt5'
.This PR thus avoids changing
os.environ
at all until a suitable API has been chosen, and only then (in theelse
clause of thetry
s) setos.environ[QT_API]
.An alternative would be not to modify
os.environ
at all, but this seems less than ideal because it will make it so that the spawned process will not necessarily end up with the same Qt binding, depending on whether or not that process does animport PyQt6
andimport qtpy
in the same order as the main process, at least in the case where a user has two bindings (e.g., PyQt5 and PyQt6).This PR also promotes a
assert
statement to a proper and more informativeraise ValueError(...)
(that also will not be skipped in Python optimized mode), and changes some'QT_API'
s toQT_API
, as it seems like that's what the variable name is for. But if either of these is not actually desirable, I can revert!